Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(lib): add additive behavior #82

Closed
wants to merge 2 commits into from
Closed

Conversation

vascocc
Copy link

@vascocc vascocc commented Sep 3, 2019

This commit adds additive behavior, mimicking the behavior when holding the Shift key.
Updated app to be able to select addictive behavior.
Updated Cypress tests.
Updated documentation.

@d3lm
Copy link
Owner

d3lm commented Oct 4, 2019

Hey there! Sorry for my late response. I have now looked at your PR and it looks good so far. My question is, how is the "additive" mode supposed to work with drag to select disabled. So when this option is false then the new additive mode doesn't work. Is that our intention? I as a user would expect that when additive is true then this will always add to the current selection, unless I press the key combo for subtracting from the current selection. Curious to hear your thoughts on this.

For select mode it makes sense that additive is not working, cause that doesn't really make sense I guess. I am also wondering how we should properly document the possible configuration options or how this could be properly handled in code so that we don't even allow impossible combinations. Any idea?

At the end of the day, I think additive should work with select on drag to be true and false.

@d3lm
Copy link
Owner

d3lm commented Oct 21, 2019

@vascocc Quick heads-up for you: I am awaiting #89 to be merged before I ll merge this one. I suppose you'll have to rebase then on top of latest master once it lands. It's an essential feature and that together with your PR will go into one new release.

@vascocc
Copy link
Author

vascocc commented Oct 21, 2019

@d3lm, ok, no problem. I didn't look at the code again to answer your first comment. I'll try to do that in the next week or so.

@d3lm
Copy link
Owner

d3lm commented Oct 21, 2019

Thanks!

@d3lm
Copy link
Owner

d3lm commented Oct 25, 2019

Alright, #89 was merged. Do you mind updating this PR and rebasing it on top of latest master? Make sure all tests work as expected. Then I ll take another look and review your PR to get this in.

@vascocc
Copy link
Author

vascocc commented Oct 25, 2019

@d3lm sure, no problem.

@vascocc
Copy link
Author

vascocc commented Nov 4, 2019

@d3lm, just rebased this branch and started thinking about drag to select and additive together.
When I opened the issue my objective was to add and subtract without using the shortcut keys.
So, I will suggest the following:

  • If additive is true, when clicked an unselected cell it becomes selected and if the cell is selected it becomes unselected;
  • If additive is true, when dragging through unselected cells they will become selected and if selected they will become unselected;
  • The additive behavior (when dragging) should be the same as drag to select;

Regarding impossible combinations:

  • disable overrides the other configurations;
  • select mode overrides dragging configurations like selectOnDrag, disableDrag and additive;

Let me know what do you think.

@d3lm
Copy link
Owner

d3lm commented Nov 4, 2019

@vascocc Thanks! I do agree with what you said, tho the branch still has conflicts and cannot be rebased on latest master. Mind checking that out and resolving the issues?

@d3lm
Copy link
Owner

d3lm commented Nov 4, 2019

Also, your changes include changes from previous commits which is a bit strange. It's hard to review the PR. That should normally not be the case 🤔

@vascocc
Copy link
Author

vascocc commented Nov 7, 2019

I was using another computer with different tools for managing Git, maybe I messed up the rebase. I'll take a look at it.

@vascocc
Copy link
Author

vascocc commented Nov 7, 2019

@d3lm, I really messed up the rebase. I think this branch it's beyond recovery. If you don't mind I can start again and do a new pull request.

@d3lm
Copy link
Owner

d3lm commented Nov 8, 2019

Yea I think that would be nice if you started with a new PR, cause this one is now pretty messy and has changes from other PRs. But you can of course just take your code and copy it over into a new PR.

Appreciate your work on this one!

This commit adds additive behavior, mimicking the behavior when holding the Shift key.
Updated app to be able to select addictive behavior.
Updated Cypress tests.
Updated documentation.
@d3lm
Copy link
Owner

d3lm commented Nov 28, 2019

I just wanted to ask if there is any update on this one, but I must have missed your force push into this PR. I ll take a look at it.

@d3lm
Copy link
Owner

d3lm commented Nov 29, 2019

Hey @vascocc, so I have checked out this PR on my machine but I cannot verify the behavior you have described above:

If additive is true, when clicked an unselected cell it becomes selected and if the cell is selected it becomes unselected;

For me, when additive is activated and I select an item it stays selected even though it's not within the bounding box anymore while dragging. Also, a selected item is not unselected.

If additive is true, when dragging through unselected cells they will become selected and if selected they will become unselected;

This is also something I cannot verify because items for me never get unselected.

The additive behavior (when dragging) should be the same as drag to select;

This is not the case for me, as described above.

Do you mind elaborating on this?

@vascocc
Copy link
Author

vascocc commented Nov 30, 2019

I just wanted to ask if there is any update on this one, but I must have missed your force push into this PR. I ll take a look at it.

I'm working on a new branch. Hope I can finish this weekend. As I mentioned before, I'm going to close this PR and open a new one. Sorry for the delay.

I'll check those issues too.

@d3lm
Copy link
Owner

d3lm commented Nov 30, 2019

Got it! Thanks 🙏

This commit adds additive behavior, mimicking the behavior when holding the Shift key.
Fixed Additive implementation;
Updated tests and documentation;
@vascocc
Copy link
Author

vascocc commented Dec 1, 2019

Closing this PR.

@vascocc vascocc closed this Dec 1, 2019
@vascocc
Copy link
Author

vascocc commented Dec 1, 2019

New PR at #94

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants